chore: sync upstream 2026-09-13 - #77
Conversation
…quester retries below the turn (MoonshotAI#3734)
|
❌ Nix build failed Hash mismatch in
Please update |
![zuraffa-review[bot]](https://avatars.githubusercontent.com/in/4884899?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR review — chore: sync upstream 2026-09-13 @ 8394b6ee
Fork sync merging upstream MoonshotAI/kimi-code@ee2cac102 (fix(agent-core-v2): invalidate streamed attempt state when the llm requester retries below the turn, MoonshotAI#3734) into fork master, resolving the three conflicting llmRequester* files additively.
Note: this PR merged (as b3275579) while the review was in flight — the review is pinned to the reviewed commit 8394b6ee; findings below are informational and unblocked.
Merge audit — verified against the real upstream commit
| Check | Result |
|---|---|
| Upstream scope | upstream ee2cac102 touches the same 12 files (llmRequester.ts, llmRequesterService.ts, loop/machine/requester.ts, loop/machine/tools.ts, human/agent/turn.ts, llm/requester/requester.ts, model-requester-impl.ts, both llm.md docs, .changeset file, 2 test files) |
| Upstream hunks preserved | 9/12 files are byte-identical to upstream's post-commit state; the 3 diverging files are exactly the conflicts, and every divergence is fork-owned content (model override, substitute/fallback cascade, fallback tests) — no upstream change was dropped |
| Fork code preserved | model?: string, substituteModelActiveKey, activateFallback + terminal-400 fallback all intact; the fallback path is wired into the new onAttemptRetry contract (llmRequesterService.ts:496) and asserted by the fork's own fallback test |
| Fork-owned-file guardrail | re-ran it independently on the merge commit: 31 entries, 0 missing files, 0 missing markers |
| Regression coverage | the new turn-level test reproduces "partial tool call streamed → request retried below the turn" and asserts only the surviving attempt's call id; machineTools.test.ts covers duplicate-id supersede + onBatchError |
Findings
🔴 0 · 🟠 0 · 🟡 0 · 🔵 3 (all non-blocking, inline on the diff):
- 🔵
packages/agent-core-v2/src/agent/loop/machine/tools.ts:144—startBatch's.catchis unreachable in practice. - 🔵
packages/agent-core-v2/src/agent/llmRequester/llmRequesterService.ts:340— redundantonAttemptRetryparameter in the mergedrunRequestshape. - 🔵
packages/agent-core-v2/src/llm-adapter/model/model-requester-impl.ts:216-222— the newllm.request.retryingreset has no producer at that layer today.
Out-of-diff note — the guardrail follow-up claim looks incorrect on CI
The PR body says the fork-owned-file guardrail "cannot parse .github/FORK_OWNED_FILES's padded path :: marker format — its sed leaves trailing whitespace, so every entry reads as missing and the guardrail would fail the next clean sync."
- I reproduced the described failure on macOS: BSD
seddoes not implement\+in a BRE, sosed -e 's/[[:space:]]\+::.*$//'does not match a padded line — the "path" keeps its padding and the::markertail, and[ -f "$path" ]fails. That is exactly the symptom in the note. - However the workflow runs on
ubuntu-latest(.github/workflows/sync-upstream.yml:31), i.e. GNUsed, where\+is the documented GNU BRE extension for "one or more":[[:space:]]\+matches the whole run of spaces and the padded format parses correctly there. - So on CI the guardrail is not broken; the only real nit is portability if that step is ever run locally on macOS —
[[:space:]][[:space:]]*(or[[:space:]]\{1,\}) makes it platform-proof. Worth a quick re-check on the runner before changing the workflow, so a working guardrail is not "fixed" into something different.
For the fixing agent: the three 🔵 inline items above plus this guardrail-diagnosis note are the complete findings list; all optional, nothing blocks.
| }; | ||
|
|
||
| const startBatch = (entries: readonly PendingEntry[]): void => { | ||
| void runBatch(entries).catch((error: unknown) => { |
There was a problem hiding this comment.
🔵 Nitpick (⚡) — startBatch's .catch is unreachable in practice: runBatch already reports through options.onBatchError in its own catch (tools.ts:132-137) and never rethrows, so the promise only rejects if onBatchError itself throws — and then the same error gets reported twice. If you want a single report path, drop this wrapper (or rethrow in runBatch's catch and report only here). It is upstream's line, so leaving it as-is is also a reasonable call.
| onPart: AgentLLMRequestPartHandler, | ||
| signal: AbortSignal | undefined, | ||
| onRequestTrace: (traceId: string | undefined) => void, | ||
| onAttemptRetry: (() => void) | undefined, |
There was a problem hiding this comment.
🔵 Nitpick (⚡) — in the fork's merged shape this parameter duplicates overrides.onAttemptRetry: runRequest already receives overrides (line 336) and the only call site passes exactly overrides.onAttemptRetry (line 268). Upstream needed the separate parameter because its runRequest takes a resolved request; here const onAttemptRetry = overrides.onAttemptRetry; at the top of the body removes the parallel argument. Low priority — keeping it to stay textually close to upstream is defensible.
| case 'llm.request.retrying': { | ||
| accumulator = createMessageAccumulator(); | ||
| usage = undefined; | ||
| finish = undefined; | ||
| messageId = undefined; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔵 Observation (⚡) — at this commit nothing emits llm.request.retrying at this layer: the loop-machine requester (agent/loop/machine/requester.ts:86) is the only emitter and it sits above ModelRequesterImpl, while the base protocol requesters (human/llm/requester/bases/*/requester.ts) emit llm.sent but never this event — so this reset currently cannot fire (each service-level retry re-enters request() with a fresh local accumulator anyway). Harmless and plausibly forward-looking; flagging so nobody debugs a stale streamed state expecting this branch to have run.
|
Walkthrough — What this PR does: merges upstream How it was reviewed:
Outcome: 0 🔴 / 0 🟠 / 0 🟡 / 3 🔵 — the three nitpicks are posted inline and are optional cleanups; nothing blocks. One out-of-diff note: the PR body's guardrail-parsing follow-up looks incorrect for CI (the ubuntu runner's GNU Note: the PR merged as |
|
Thanks — the guardrail note is correct and my diagnosis was wrong. Re-checking the committed bytes ( I've corrected PR #78's description: the change (POSIX On the three inline items:
|
2 participants
Related Issue
Resolve #75
(Issue #76 is the catch-all failure issue opened by the same sync run — the only failing step was the conflict abort below, so it has no separate cause.)
Problem
The daily upstream sync could not auto-merge upstream
ee2cac102("invalidate streamed attempt state when the llm requester retries below the turn") into forkmaster. Three fork-owned files conflicted:packages/agent-core-v2/src/agent/llmRequester/llmRequester.tspackages/agent-core-v2/src/agent/llmRequester/llmRequesterService.tspackages/agent-core-v2/test/agent/llmRequester/llmRequesterService.test.tsBoth sides had diverged in the same interfaces: upstream added an
onAttemptRetrycallback for discarding an interrupted attempt's streamed state, while the fork carries themodelrequest override and the terminal-400 fallback cascade.What changed
Merged upstream, resolving the three conflicts additively so no fork-owned code was dropped:
AgentLLMRequestOverridesnow carries both the fork'smodeloverride and upstream'sonAttemptRetry.runRequestkeeps the fork's internalresolveRequest(overrides)call (it must re-resolve afteractivateFallback('terminal-error')) and additionally threads upstream'sonAttemptRetry.onAttemptRetry, so the interrupted attempt's streamed state is discarded when the fallback model takes over — the same below-turn retry case upstream's fix targets. Without this, a partial tool call streamed before the 400 could leak into the fallback attempt's message.onAttemptRetryfires on fallback) and upstream's attempt-retry / turn-machine stream-state tests.Verification
vitest run test/agent/llmRequester/llmRequesterService.test.ts— 46/46 pass (fork fallback and upstream attempt-retry suites together).vitest run test/agent/loop(including upstream's newmachineTools.test.ts) — passes exceptloop.test.ts, which fails identically on pristinemasterin the same environment.agent-core-v2suite compared against a pristinemasterworktree: identical genuine failures (pre-existing/environmental). The 8 files that differed on a first pass all passed on re-run..github/FORK_OWNED_FILESguardrail: 31 entries, 0 missing, 0 markers dropped —activateFallback,substituteModelActiveKey, andisTerminalProviderApiErrorall survive.Follow-up (not changed here)
The fork-owned-file guardrail in
.github/workflows/sync-upstream.ymlcannot parse.github/FORK_OWNED_FILES's paddedpath :: markerformat — itssedleaves trailing whitespace, so every entry reads as missing and the guardrail would fail the next clean sync. Worth fixing separately.Checklist
gen-changesetsskill, or this PR needs no changeset. (No new changeset: upstream's.changeset/fix-infinite-retry-stream-invalidation.mdalready covers the user-facing effect.)gen-docsskill, or this PR needs no doc update. (Upstream'sdocs/{en,zh}/llm.mdupdates are included in the merge.)